Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set Kubelet Disk Defaults for the 1.7 release #46448

Merged
merged 1 commit into from
Jun 1, 2017
Merged

Set Kubelet Disk Defaults for the 1.7 release #46448

merged 1 commit into from
Jun 1, 2017

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented May 25, 2017 •
edited

The --low-diskspace-threshold-mb flag has been depreciated since 1.6.
This PR sets the default to 0, and sets defaults for disk eviction based on the values used for our e2e tests.
This also removes the custom defaults for vagrant, as the new defaults should work for it as well.

/assign @derekwaynecarr
cc @vishh

By default, --low-diskspace-threshold-mb is not set, and --eviction-hard includes "nodefs.available<10%,nodefs.inodesFree<5%"

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 25, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels May 25, 2017
@dashpole
Copy link
Contributor Author

/release-note

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 25, 2017
@dashpole
Copy link
Contributor Author

/assign @pwittrock @vishh

@dashpole
Copy link
Contributor Author

@k8s-bot pull-kubernetes-unit test this

@dashpole
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@pwittrock pwittrock removed their assignment May 26, 2017
@dashpole
Copy link
Contributor Author

@pwittrock i needed an approver from hack/OWNERS

@derekwaynecarr
Copy link
Member

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017
@derekwaynecarr derekwaynecarr removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017
derekwaynecarr
derekwaynecarr reviewed May 26, 2017
View reviewed changes
Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question on why % versus literal.

pkg/apis/componentconfig/v1alpha1/defaults.go
@@ -396,7 +393,7 @@ func SetDefaults_KubeletConfiguration(obj *KubeletConfiguration) {
obj.HairpinMode = PromiscuousBridge
}
if obj.EvictionHard == nil {
temp := "memory.available<100Mi"
temp := "memory.available<100Mi,nodefs.available<10%,nodefs.inodesFree<5%"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, quick question here... why make this a percentage and not a literal value same as the old lowDiskSpace default of 256MB?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use a literal value here as well if you want. If we use 256MB, I have a couple questions:
Should we enable inode eviction? If so, is a percentage still appropriate?
Should I still remove the custom defaults for vagrant, since they used nodefs.available<10% previously?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think for a default, 10% is still probably fine. re-applying my original tag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For storage I think performance is relative to available percentage of capacity and not an absolute value. so a percentage here makes sense

@derekwaynecarr derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017
@derekwaynecarr
Copy link
Member

/approve

@dashpole
Copy link
Contributor Author

@k8s-bot pull-kubernetes-verify test this

@dashpole
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

1 similar comment
@dashpole
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

vishh
vishh approved these changes May 31, 2017
View reviewed changes
Copy link
Contributor

@vishh vishh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@dashpole
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@dchen1107
Copy link
Member

/lgtm

@dchen1107
Copy link
Member

Thanks for the cleanup.

@vishh
Copy link
Contributor

vishh commented Jun 1, 2017

/assign vishh

@vishh
Copy link
Contributor

vishh commented Jun 1, 2017

Not sure why the bot fails to apply approved label

@vishh vishh added approved Indicates a PR has been approved by an approver from all required OWNERS files. approved-for-milestone labels Jun 1, 2017
@vishh vishh added this to the v1.7 milestone Jun 1, 2017
@dashpole
Copy link
Contributor Author

dashpole commented Jun 1, 2017

arg... the test job keeps failing because we are out of quota for subnetworks...

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dchen1107, derekwaynecarr

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@dchen1107
Copy link
Member

Quota issue should be fixed.

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@dashpole
Copy link
Contributor Author

dashpole commented Jun 1, 2017

@k8s-bot pull-kubernetes-unit test this

@dashpole
Copy link
Contributor Author

dashpole commented Jun 1, 2017

@k8s-bot pull-kubernetes-node-e2e test this

@dashpole
Copy link
Contributor Author

dashpole commented Jun 1, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 62435ed into kubernetes:master Jun 1, 2017
@dashpole dashpole deleted the disk_eviction_defaults branch June 14, 2017 19:26
@dashpole dashpole mentioned this pull request Jul 12, 2017
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekwaynecarr derekwaynecarr derekwaynecarr left review comments

@vishh vishh vishh approved these changes

Assignees

@derekwaynecarr derekwaynecarr

@vishh vishh

@dchen1107 dchen1107

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dashpole @derekwaynecarr @dchen1107 @vishh @k8s-github-robot @pwittrock @k8s-ci-robot